Move effect-checking to MIR #44700

arielb1 · 2017-09-19T13:24:47Z

This allows emitting lints from MIR and moves the effect-checking pass to work on it.

I'll make repr(packed) misuse unsafe in a separate PR.

eddyb · 2017-09-19T14:25:58Z

LGTM. Unsure about having a separate vector for visibility scope information (and I'd rename them to "lexical" or "source" scopes). r? @nikomatsakis

bjorn3 · 2017-09-19T14:32:23Z

src/librustc/mir/mod.rs

+
+impl<T> serialize::Decodable for ClearOnDecode<T> {
+    fn decode<D: serialize::Decoder>(d: &mut D) -> Result<Self, D::Error> {
+        serialize::Decodable::decode(d).map(|_v: ()| ClearOnDecode::Clear)


.map(|()| ...) is possible here. (() matches the only val of type ()

kennytm · 2017-09-19T16:19:16Z

src/librustc/ich/impls_mir.rs

@@ -75,6 +76,22 @@ for mir::Terminator<'gcx> {
    }
 }

+impl<'a, 'gcx, 'tcx, T> HashStable<StableHashingContext<'a, 'gcx, 'tcx>> for mir::ClearOnDecode<T>


StableHashingContext now only takes one lifetime: StableHashingContext<'gcx>.

bors · 2017-09-20T01:07:46Z

☔ The latest upstream changes (presumably #44505) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2017-09-20T13:12:53Z

src/librustc/mir/mod.rs

+    /// The *lexical* visibility scope the local is defined
+    /// in. If the local was defined in a let-statement, this
+    /// is *within* the let-statement, rather than outside
+    /// of iit.


(typo, "iit")

hate that keyboard. should get a new one that is actually good.

pnkfelix · 2017-09-20T13:14:39Z

src/librustc_driver/driver.rs

@@ -1004,15 +1004,16 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
    // FIXME: ariel points SimplifyBranches should run after


... we can remove the FIXME as part of this PR, right?

pnkfelix · 2017-09-20T15:43:57Z

(LGTM too.)

nikomatsakis

This looks pretty awesome. r=me but I'm not sure what's up with the THIS IS A TEST comment.

nikomatsakis · 2017-09-20T21:22:43Z

src/librustc_mir/transform/type_check.rs

@@ -487,6 +487,16 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {

                if self.is_box_free(func) {
                    self.check_box_free_inputs(mir, term, &sig, args);
+                    // THIS IS A TEST. TEST TEST.


what does this comment mean?

I wanted to see that lints worked before I actually used them to implement effect checking.

oops. this should never have reached github. removing.

arielb1 · 2017-09-20T21:30:11Z

@bors r=nikomatsakis

bors · 2017-09-20T21:30:11Z

📌 Commit 48466b7 has been approved by nikomatsakis

bors · 2017-09-22T20:04:40Z

⌛ Testing commit 48466b75cf16294f1d3bef913f2ac4c34cb80a94 with merge 3a3144bc996f5950bc3415dd6c7325ed87ebbd2c...

bors · 2017-09-22T21:17:12Z

💔 Test failed - status-appveyor

kennytm · 2017-09-23T05:42:29Z

src/test/compile-fail/issue-43733.rs

@@ -16,6 +16,8 @@ type Foo = std::cell::RefCell<String>;
 #[cfg(target_thread_local)]
 static __KEY: std::thread::__FastLocalKeyInner<Foo> =
    std::thread::__FastLocalKeyInner::new();
+//~^^ ERROR Sync` is not satisfied
+//~^^^ ERROR Sync` is not satisfied


check x86_64-pc-windows-gnu failed, can't find these two errors.

Windows aren't cfg(target_thread_local) IIRC.

---- [compile-fail] compile-fail\issue-43733.rs stdout ---- error: C:/projects/rust/src/test/compile-fail/issue-43733.rs:17: expected error not found: Sync` is not satisfied error: C:/projects/rust/src/test/compile-fail/issue-43733.rs:17: expected error not found: Sync` is not satisfied error: 0 unexpected errors found, 2 expected errors not found

Only mingw windows. MSVC windows does have cfg(target_thread_local).
However... this error shouldn't be there, I think I forgot to put #[thread_local] on it when I wrote the test? Or maybe I wrote the test, then later changed #[thread_local] and forgot to update the test (because it didn't give these errors because MIR checks run far too late :/).

Anyway, just adding #[thread_local] should work.

No functional changes intended.

arielb1 · 2017-09-24T10:15:50Z

@bors r=nikomatsakis

bors · 2017-09-24T10:15:51Z

📌 Commit 516534f has been approved by nikomatsakis

bors · 2017-09-25T00:52:22Z

⌛ Testing commit 516534f with merge 7a9cdc4...

@eddyb

Move effect-checking to MIR This allows emitting lints from MIR and moves the effect-checking pass to work on it. I'll make `repr(packed)` misuse unsafe in a separate PR. r? @eddyb

bors · 2017-09-25T05:05:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 7a9cdc4 to master...

rust-highfive assigned eddyb Sep 19, 2017

arielb1 force-pushed the mir-effectck branch from 459e6ca to f64623c Compare September 19, 2017 13:26

rust-highfive assigned nikomatsakis and unassigned eddyb Sep 19, 2017

bjorn3 requested changes Sep 19, 2017

View reviewed changes

kennytm requested changes Sep 19, 2017

View reviewed changes

arielb1 force-pushed the mir-effectck branch from f64623c to 09030aa Compare September 19, 2017 17:33

arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2017

arielb1 force-pushed the mir-effectck branch from 09030aa to 9be1ef7 Compare September 20, 2017 10:43

pnkfelix reviewed Sep 20, 2017

View reviewed changes

nikomatsakis approved these changes Sep 20, 2017

View reviewed changes

arielb1 force-pushed the mir-effectck branch 2 times, most recently from f4b3b24 to 48466b7 Compare September 20, 2017 21:29

nikomatsakis added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2017

kennytm reviewed Sep 23, 2017

View reviewed changes

arielb1 added 5 commits September 24, 2017 12:46

add lint levels to VisibilityScope

8c7500f

move unsafety checking to MIR

c72a979

No functional changes intended.

address review comments

9c4898e

remove test code accidentally checked in

b408144

fix test

516534f

arielb1 force-pushed the mir-effectck branch from 48466b7 to 516534f Compare September 24, 2017 10:15

bors mentioned this pull request Sep 25, 2017

The Magenta kernel is now called Zircon. #44612

Merged

bors merged commit 516534f into rust-lang:master Sep 25, 2017

hanna-kruppe mentioned this pull request Oct 8, 2017

Nightly: unsafe code is allowed outside unsafe blocks when the code is unreachable #45087

Closed

bluss mentioned this pull request Oct 8, 2017

Missing unnecessary unsafe block warning #45107

Closed

matthewjasper mentioned this pull request Jan 13, 2018

Matching on uninhabited unsafe places (union fields, raw pointer dereferences, etc.) allowed in safe code. #47412

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move effect-checking to MIR #44700

Move effect-checking to MIR #44700

arielb1 commented Sep 19, 2017 •

edited

eddyb commented Sep 19, 2017

bjorn3 Sep 19, 2017

kennytm Sep 19, 2017

bors commented Sep 20, 2017

pnkfelix Sep 20, 2017

arielb1 Sep 20, 2017

pnkfelix Sep 20, 2017

pnkfelix commented Sep 20, 2017

nikomatsakis left a comment

nikomatsakis Sep 20, 2017

arielb1 Sep 20, 2017

arielb1 Sep 20, 2017 •

edited

arielb1 commented Sep 20, 2017

bors commented Sep 20, 2017

bors commented Sep 22, 2017

bors commented Sep 22, 2017

kennytm Sep 23, 2017

eddyb Sep 23, 2017

arielb1 commented Sep 24, 2017

bors commented Sep 24, 2017

bors commented Sep 25, 2017

bors commented Sep 25, 2017

		@@ -1004,15 +1004,16 @@ pub fn phase_3_run_analysis_passes<'tcx, F, R>(sess: &'tcx Session,
		// FIXME: ariel points SimplifyBranches should run after

Move effect-checking to MIR #44700

Move effect-checking to MIR #44700

Conversation

arielb1 commented Sep 19, 2017 • edited

eddyb commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Sep 20, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Sep 20, 2017 • edited

Choose a reason for hiding this comment

arielb1 commented Sep 20, 2017

bors commented Sep 20, 2017

bors commented Sep 22, 2017

bors commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 commented Sep 24, 2017

bors commented Sep 24, 2017

bors commented Sep 25, 2017

bors commented Sep 25, 2017

arielb1 commented Sep 19, 2017 •

edited

arielb1 Sep 20, 2017 •

edited